Add independent ciper and MAC algorithms negotiation for each direction by yosuke-wolfssl · Pull Request #952 · wolfSSL/wolfssh

yosuke-wolfssl · 2026-04-24T00:22:56Z

This PR adds independent ciper and MAC algorithms negotiation for each direction to comply with RFC 4253 section 7.1.

Add peerEncryptId, peerMacId, peerAeadMode, peerBlockSz, peerMacSz into HandshakeInfo.
Fix DoKexInit() so that this matches each algorithm independently and store the results into each member in HandshakeInfo.
Fix DoNewKeys() so that this updates peer algorithm info like ssh->peerEncryptId with ssh->handshake->peerEncryptId and so on.

Also, new regress test for the case is added as well.

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds RFC 4253 §7.1-compliant, per-direction cipher/MAC negotiation by tracking and applying independent peer vs local (C2S vs S2C) algorithm selections during KEXINIT/NEWKEYS processing, with a new regression test covering mixed-direction negotiation scenarios.

Changes:

Extend HandshakeInfo to store peer (opposite direction) cipher/MAC/AEAD and sizes independently.
Update DoKexInit() to negotiate cipher/MAC independently per direction and populate the new handshake fields.
Update DoNewKeys() to copy negotiated peer-direction algorithm selections from the handshake into the session, and add a regression test validating negotiation outcomes.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
`wolfssh/internal.h`	Adds per-direction (peer) negotiated cipher/MAC/AEAD fields to `HandshakeInfo`.
`src/internal.c`	Implements independent per-direction algorithm matching and propagates peer settings on `NEWKEYS`.
`tests/regress.c`	Adds a regression test ensuring negotiation differs per direction and handles AEAD vs non-AEAD correctly.

Comments suppressed due to low confidence (1)

src/internal.c:4486

The KEXINIT parsing now duplicates the same 'convert configured algo name-list into ID list' pattern multiple times (cipher S2C, MAC C2S, MAC S2C). Consider extracting a small helper to build cannedList/cannedListSz from ssh->algoListCipher / ssh->algoListMac to reduce repetition and the chance of future divergence between the per-direction negotiation blocks.

    if (ret == WS_SUCCESS && !ssh->handshake->peerAeadMode) {
        cannedAlgoNamesSz = AlgoListSz(ssh->algoListMac);
        cannedListSz = (word32)sizeof(cannedList);
        ret = GetNameListRaw(cannedList, &cannedListSz,
                (const byte*)ssh->algoListMac, cannedAlgoNamesSz);

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

wolfSSL-Fenrir-bot

Fenrir Automated Review — PR #952

Scan targets checked: wolfssh-bugs, wolfssh-src

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

aidangarske

Skoll Code Review

Scan type: review-security
Overall recommendation: REQUEST_CHANGES
Findings: 3 total — 3 posted, 0 skipped
3 finding(s) posted as inline comments (see file-level comments below)

Posted findings

[High] GenerateKeys skips both-direction MAC key derivation when only one direction is AEAD — src/internal.c:2696-2709
[Low] Test coverage asserts DoKexInit parse state only, not key derivation / data path for split-AEAD — tests/regress.c:2142-2210
[Info] HandshakeInfoNew skips peerMacSz/peerAeadMode initialization (safe due to WMEMSET but easy to miss) — src/internal.c:572-590

Review generated by Skoll

wolfSSL-Fenrir-bot

Fenrir Automated Review — PR #952

Scan targets checked: wolfssh-bugs, wolfssh-src

Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

wolfSSL-Fenrir-bot

Fenrir Automated Review — PR #952

Scan targets checked: wolfssh-bugs, wolfssh-src

No new issues found in the changed files. ✅

yosuke-wolfssl · 2026-04-27T01:18:30Z

Hello @aidangarske ,

I fixed the issues you mentioned.
Could you please review this again ?
Thank you for your time.

aidangarske

Skoll Multi-Scan Review

Modes: review + review-security + audit
Overall recommendation: COMMENT
Findings: 4 total — 4 posted, 0 skipped
4 finding(s) posted as inline comments (see file-level comments below)

Posted findings

[Medium] [audit] DoKexInit client-side branch (WOLFSSH_ENDPOINT_CLIENT alias mapping) lacks a direct asymmetric-algo test — src/internal.c:4313-4326
[Low] [review+review-security] Asymmetric AEAD reset between C2S and S2C branches in DoKexInit — src/internal.c:4427-4478
[Low] [review-security] Pointer aliases used after first ret==WS_SUCCESS block may trip strict-uninit warnings — src/internal.c:4253-4326,4427-4527
[Info] [review-security] Removed redundant ID_NONE/MSGID_NONE initializations in HandshakeInfoNew rely on enum values being 0 — src/internal.c:572-576

Review generated by Skoll

…on, And add regress test

yosuke-wolfssl · 2026-04-28T02:45:07Z

Hi @aidangarske ,

Sorry to take your time.
Can you review again please ?

aidangarske

Skoll Multi-Scan Review

Modes: audit + review + review-security
Overall recommendation: COMMENT
Findings: 7 total — 7 posted, 0 skipped
6 finding(s) posted as inline comments (see file-level comments below)

Posted findings

[Medium] [audit] Asymmetric S2C-only encryption-algo mismatch error path is untested — src/internal.c:4448-4466
[Medium] [audit] Asymmetric S2C-only MAC-algo mismatch error path is untested — src/internal.c:4510-4533
[Medium] [review] TestIndependentAlgoNegotiation tests do not verify all ID/Sz fields are reset on each ssh instance — tests/regress.c:2161-2332
[Low] [audit] wolfSSH_TestGenerateKeys NULL-argument validation is untested — src/internal.c:17962-17967
[Low] [review] Missing blank line between TestGenerateKeysSplit and TestGenerateKeysSplitClient — tests/regress.c:2446-2447
[Low] [review] New tests silently ignore wolfSSH_TestDoKexInit return without explanation — tests/regress.c:2191,2227,2278,2313,2366,2420,2479,2532
[Low] [review] DoKexInit reads side from ssh->ctx->side a second time after caching it in side — src/internal.c:4622,4642,4666,4673

Review generated by Skoll

aidangarske · 2026-04-28T16:34:31Z

+        }
+    }

    /* Enc Algorithms - Server to Client */


🟠 [Medium] Asymmetric S2C-only encryption-algo mismatch error path is untested · path

Pre-PR, the S2C encryption algorithm was matched against the C2S-chosen algorithm only (MatchIdLists(side, list, listSz, &algoId, 1)). Post-PR, S2C is matched independently against ssh->algoListCipher via cannedList. This introduces a NEW failure mode: C2S successfully matches but S2C does not (or vice versa) — yielding WS_MATCH_ENC_ALGO_E from the S2C block. The new tests in TestIndependentAlgoNegotiation/...Client only cover the success paths for asymmetric cipher selection; no test exercises asymmetric mismatch where one direction agrees and the other fails. Grep confirms WS_MATCH_ENC_ALGO_E has zero references in tests/.

Fix: Add a regress.c test that builds a kex_init payload where the C2S cipher list contains an algorithm in the local algoListCipher but the S2C cipher list does not (e.g., local accepts aes128-cbc,aes256-ctr; payload offers C2S=aes128-cbc, S2C=3des-cbc). Verify wolfSSH_TestDoKexInit returns WS_MATCH_ENC_ALGO_E. Mirror the test for the inverse case (C2S fails, S2C succeeds).

aidangarske · 2026-04-28T16:34:31Z

+                s2cKeys->macKeySz  = KeySzForId(algoId);
+            }
        }
    }


🟠 [Medium] Asymmetric S2C-only MAC-algo mismatch error path is untested · path

The S2C MAC negotiation was rewritten from MatchIdLists(side, list, listSz, &algoId, 1) (which forced S2C to equal C2S) to an independent match against ssh->algoListMac. The new failure path WS_MATCH_MAC_ALGO_E can now trigger when C2S MAC matches but S2C MAC does not (and vice versa for the C2S branch at lines 4484-4508). Neither branch's WS_MATCH_MAC_ALGO_E return is exercised by any test. Grep across the repo confirms no test references WS_MATCH_MAC_ALGO_E.

Fix: Add a regress.c test that supplies asymmetric MAC lists where one direction shares no entries with ssh->algoListMac (e.g., local accepts hmac-sha1,hmac-sha2-256; payload offers C2S=hmac-sha1, S2C=hmac-md5). Assert wolfSSH_TestDoKexInit returns WS_MATCH_MAC_ALGO_E. Cover the inverse C2S-mismatch case as well.

aidangarske · 2026-04-28T16:34:31Z

    TestFirstPacketFollowsSkipped();
 }

+#if !defined(WOLFSSH_NO_AES_CBC) && !defined(WOLFSSH_NO_AES_CTR) \


🟠 [Medium] TestIndependentAlgoNegotiation tests do not verify all ID/Sz fields are reset on each ssh instance · test

The new tests only assert the split fields (peerEncryptId/peerMacId/peerAeadMode/peerBlockSz/peerMacSz) for the side they care about, but never assert peerBlockSz/peerMacSz directly. Given that the diff intentionally drops the explicit ID_NONE/MSGID_NONE initializers in HandshakeInfoNew (relying on the WMEMSET zero plus the assumption that ID_NONE == 0), an asymmetric subtle regression — e.g. peerBlockSz left at its MIN_BLOCK_SZ default in a code path that should overwrite it — would not be caught. A few additional AssertIntEQ lines for peerBlockSz, peerMacSz, blockSz, macSz after each negotiation would harden the tests against future refactors of HandshakeInfoNew or DoKexInit.

Fix: Add explicit assertions for the new peerBlockSz/peerMacSz fields (and for blockSz/macSz for symmetry) in both server and client variants.

aidangarske · 2026-04-28T16:34:31Z

    return DoKexInit(ssh, buf, len, idx);
 }

+int wolfSSH_TestGenerateKeys(WOLFSSH* ssh)


🔵 [Low] wolfSSH_TestGenerateKeys NULL-argument validation is untested · test

The newly added test helper validates ssh == NULL || ssh->handshake == NULL and returns WS_BAD_ARGUMENT, but no test in tests/regress.c (or anywhere else) calls it with NULL inputs to verify the guard. All four new test sites (TestGenerateKeysSplit, TestGenerateKeysSplitClient sub-tests A and B) invoke it only after AssertNotNull(ssh->handshake). While the function is test-only infrastructure, a sibling invariant check is cheap to add.

Fix: Add a one-line assertion in regress.c (e.g., inside or adjacent to TestGenerateKeysSplit): AssertIntEQ(wolfSSH_TestGenerateKeys(NULL), WS_BAD_ARGUMENT); and a second call with a freshly-created WOLFSSH* whose handshake has been freed/nulled to verify the second branch.

aidangarske · 2026-04-28T16:34:31Z

+#endif /* !WOLFSSH_NO_AES_GCM */
+
+    wolfSSH_CTX_free(ctx);
+}


🔵 [Low] Missing blank line between TestGenerateKeysSplit and TestGenerateKeysSplitClient · style

Every other top-level function in regress.c (including the other three new test functions added by this PR) is separated by a blank line. The closing brace of TestGenerateKeysSplit is followed immediately by the next function definition, which breaks consistency.

Fix: Add a blank line between the two functions to match the surrounding style.

aidangarske · 2026-04-28T16:34:31Z

+            "hmac-sha1",     /* C2S MAC */
+            "hmac-sha2-256", /* S2C MAC */
+            0, payload, (word32)sizeof(payload));
+    (void)wolfSSH_TestDoKexInit(ssh, payload, payloadSz, &idx);


🔵 [Low] New tests silently ignore wolfSSH_TestDoKexInit return without explanation · test

All four new tests call (void)wolfSSH_TestDoKexInit(ssh, payload, payloadSz, &idx); and then assert handshake state. The pre-existing RunFirstPacketFollowsCase (line 2088) has an explanatory comment about why the return is ignored (the tail of DoKexInit calls SendKexInit which fails for a stripped-down server with no keys). The four new test functions reuse this pattern but do not carry a similar explanation, so a future reader may mistakenly think the discarded return is a bug. For the client-side variants the rationale is even less obvious, since the privateKeyCount check does not apply to clients — the SendKexInit failure there comes from wolfSSH_SendPacket having no IO callback set up.

Fix: Add a brief comment in each new test (or factor a helper that wraps the call) explaining why the return value is intentionally discarded.

dgarske · 2026-05-05T18:20:49Z

@yosuke-wolfssl please see the Fenrir feedback.

yosuke-wolfssl self-assigned this Apr 24, 2026

Copilot AI review requested due to automatic review settings April 24, 2026 00:22

Copilot AI reviewed Apr 24, 2026

View reviewed changes

Comment thread src/internal.c

Comment thread src/internal.c

Copilot started reviewing on behalf of yosuke-wolfssl April 24, 2026 00:35 View session

yosuke-wolfssl force-pushed the f_607 branch 2 times, most recently from 5cc90fa to 47c9e57 Compare April 24, 2026 00:59

yosuke-wolfssl assigned wolfSSL-Bot and unassigned yosuke-wolfssl Apr 24, 2026

padelsbach requested a review from wolfSSL-Fenrir-bot April 24, 2026 17:11

wolfSSL-Fenrir-bot reviewed Apr 24, 2026

View reviewed changes

aidangarske reviewed Apr 24, 2026

View reviewed changes

Comment thread src/internal.c

Comment thread tests/regress.c

Comment thread src/internal.c Outdated

yosuke-wolfssl requested a review from wolfSSL-Fenrir-bot April 26, 2026 23:08

wolfSSL-Fenrir-bot reviewed Apr 26, 2026

View reviewed changes

Comment thread src/internal.c Outdated

Comment thread src/internal.c Outdated

yosuke-wolfssl commented Apr 26, 2026

View reviewed changes

Comment thread src/internal.c Outdated

yosuke-wolfssl commented Apr 27, 2026

View reviewed changes

Comment thread src/internal.c Outdated

yosuke-wolfssl force-pushed the f_607 branch from 47c9e57 to 81a22a2 Compare April 27, 2026 01:01

yosuke-wolfssl requested a review from wolfSSL-Fenrir-bot April 27, 2026 01:02

wolfSSL-Fenrir-bot reviewed Apr 27, 2026

View reviewed changes

aidangarske reviewed Apr 27, 2026

View reviewed changes

Comment thread src/internal.c

Comment thread src/internal.c

Comment thread src/internal.c Outdated

Comment thread src/internal.c

Add independent ciper and MAC algorithms negotiation for each directi…

c514c19

…on, And add regress test

yosuke-wolfssl force-pushed the f_607 branch from 81a22a2 to c514c19 Compare April 27, 2026 23:12

aidangarske reviewed Apr 28, 2026

View reviewed changes

dgarske assigned yosuke-wolfssl May 5, 2026

Conversation

yosuke-wolfssl commented Apr 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #952

Uh oh!

aidangarske left a comment

Choose a reason for hiding this comment

Skoll Code Review

Posted findings

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #952

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #952

Uh oh!

yosuke-wolfssl commented Apr 27, 2026

Uh oh!

aidangarske left a comment

Choose a reason for hiding this comment

Skoll Multi-Scan Review

Posted findings

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yosuke-wolfssl commented Apr 28, 2026

Uh oh!

aidangarske left a comment

Choose a reason for hiding this comment

Skoll Multi-Scan Review

Posted findings

Uh oh!

aidangarske Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

aidangarske Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

aidangarske Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

aidangarske Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

aidangarske Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

aidangarske Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

dgarske commented May 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants